Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add nrfx config files to custom directory #314

Merged
merged 3 commits into from
Jan 26, 2024

Conversation

henri-gruender
Copy link
Contributor

Moving the nrfx config files allows a cleaner implementation of configuration changes and prepares for the use of a custom nrfx_glue.h.

Copy link
Member

@marbre marbre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working through this! The changes overall looks good but I would like to see that this patch is split up into multiple commits:

  • A commit adding the unmodified, imported files.
  • A commit that adjust the files to our needs or rather moves the modifications from nrf_glue_nrf5340.h to nrfx_glue.h.
  • A commit with additional changes, see additional commit.

It would be nice if every commit is able to pass the pipeline, which should be the case. This would allow to rebase and merge instead of doing a squash commit.

Comment on lines 80 to 85
${nrfx_custom_DIR}
${nrfx_DIR}
${nrfx_DIR}/drivers/include/
${nrfx_DIR}/hal/
${nrfx_DIR}/templates/
${nrfx_DIR}/mdk/
${nrfx_DIR}/soc/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems not strictly necessary as files from ${nrfx_custom_DIR} where not included for this target till now. Anyway, I am generally fine with this change but I would like to have this in a separate commit to allow an easier tracking of changes as well as understanding the motivation behind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I would rather not include it. It will be an easy change if needed.

@marbre
Copy link
Member

marbre commented Jan 19, 2024

One additional remark regarding the "import commit", we don't need to care if clang-format fails. This is one of the positive aspects of the additional work ;)

@henri-gruender
Copy link
Contributor Author

Thank you for the review @marbre. I hope the splitted commits make sense now. I believe they should all pass the pipeline.

Copy link
Member

@marbre marbre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for splitting this up and testing the commits individually!

@marbre marbre merged commit 00faca2 into iree-org:main Jan 26, 2024
8 of 9 checks passed
@henri-gruender henri-gruender deleted the nrfx-custom branch February 6, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants